Conversation
Show how to configure package repos for use with ostree image building and linux-system-roles/tox-lsr#213 Signed-off-by: Rich Megginson <rmeggins@redhat.com>
Reviewer's GuideThis PR extends get_ostree_data.sh to support collecting and formatting repository definitions in addition to packages, refactors suffix handling into a reusable list, and updates usage documentation accordingly. Class diagram for new repo formatting functions in get_ostree_data.shclassDiagram
class get_repos {
+get_repos(ostree_dir)
}
class format_repos_json {
+format_repos_json()
}
class format_repos_raw {
+format_repos_raw()
}
class format_repos_yaml {
+format_repos_yaml()
}
get_repos --> format_repos_json
get_repos --> format_repos_raw
get_repos --> format_repos_yaml
Flow diagram for new repo data collection in get_ostree_data.shflowchart TD
A["User calls get_ostree_data.sh with 'repos' argument"] --> B["get_repos() called"]
B --> C["Iterate repotype in pkgtypes"]
C --> D["Iterate suff in suffix_list"]
D --> E["Check for repos-<repotype><suff>.repo file"]
E -->|If exists| F["cat repo file contents"]
C --> G["Check for roles-<repotype>.txt file"]
G -->|If exists| H["Iterate roles in file"]
H --> I["get_rolepath for each role"]
I -->|If found| J["Recursively call get_repos on rolepath"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `.ostree/get_ostree_data.sh:141` </location>
<code_context>
+ for role in $roles; do
+ rolepath="$(get_rolepath "$ostree_dir" "$role")"
+ if [ -z "$rolepath" ]; then
+ 1>&2 echo ERROR - could not find role "$role" - please use ANSIBLE_COLLECTIONS_PATH
+ exit 2
+ fi
+ get_repos "$rolepath"
</code_context>
<issue_to_address>
**suggestion:** Error message could be more actionable for users.
Consider updating the error message to include specific steps or an example for setting ANSIBLE_COLLECTIONS_PATH, so users know how to resolve the issue.
```suggestion
1>&2 echo "ERROR - could not find role \"$role\". Please set ANSIBLE_COLLECTIONS_PATH to include the directory containing your Ansible roles. Example: export ANSIBLE_COLLECTIONS_PATH=/path/to/collections"
```
</issue_to_address>
### Comment 2
<location> `.ostree/get_ostree_data.sh:150` </location>
<code_context>
+ done
+}
+
+format_repos_json() {
+ python -c 'import sys; import json; import configparser
+cp = configparser.ConfigParser()
</code_context>
<issue_to_address>
**question:** Potential risk with shell variable expansion in repo_str.replace.
Review whether escaping dollar signs in repo_str is required, as it may affect legitimate values. If necessary, clarify the reasoning in the documentation.
</issue_to_address>
### Comment 3
<location> `.ostree/get_ostree_data.sh:177` </location>
<code_context>
+}
+
+format_repos_yaml() {
+ python -c 'import sys; import yaml; import configparser
+cp = configparser.ConfigParser()
+cp.read_file(sys.stdin)
</code_context>
<issue_to_address>
**suggestion:** yaml.safe_dump may not preserve ordering of repo fields.
If field order matters for downstream use, consider using OrderedDict or sorting keys before dumping.
Suggested implementation:
```
+ python -c 'import sys; import yaml; import configparser; from collections import OrderedDict
+cp = configparser.ConfigParser()
+cp.read_file(sys.stdin)
```
```
ret = []
# supported is from osbuild-mpp
supported = ["baseurl", "metalink", "mirrorlist",
"enabled", "metadata_expire", "gpgcheck", "username", "password", "priority",
"sslverify", "sslcacert", "sslclientkey", "sslclientcert",
"skip_if_unavailable"]
for section in cp.sections():
repo = OrderedDict()
repo["id"] = section
for option in supported:
if cp.has_option(section, option):
```
```
repo = {"id": section}
for option in supported:
if cp.has_option(section, option):
repo[option] = cp.get(section, option)
ret.append(repo)
print(yaml.safe_dump(ret, sort_keys=False))
'
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| done | ||
| } | ||
|
|
||
| format_repos_json() { |
There was a problem hiding this comment.
question: Potential risk with shell variable expansion in repo_str.replace.
Review whether escaping dollar signs in repo_str is required, as it may affect legitimate values. If necessary, clarify the reasoning in the documentation.
| } | ||
|
|
||
| format_repos_yaml() { | ||
| python -c 'import sys; import yaml; import configparser |
There was a problem hiding this comment.
suggestion: yaml.safe_dump may not preserve ordering of repo fields.
If field order matters for downstream use, consider using OrderedDict or sorting keys before dumping.
Suggested implementation:
+ python -c 'import sys; import yaml; import configparser; from collections import OrderedDict
+cp = configparser.ConfigParser()
+cp.read_file(sys.stdin)
ret = []
# supported is from osbuild-mpp
supported = ["baseurl", "metalink", "mirrorlist",
"enabled", "metadata_expire", "gpgcheck", "username", "password", "priority",
"sslverify", "sslcacert", "sslclientkey", "sslclientcert",
"skip_if_unavailable"]
for section in cp.sections():
repo = OrderedDict()
repo["id"] = section
for option in supported:
if cp.has_option(section, option):
repo = {"id": section}
for option in supported:
if cp.has_option(section, option):
repo[option] = cp.get(section, option)
ret.append(repo)
print(yaml.safe_dump(ret, sort_keys=False))
'
Show how to configure package repos for use with ostree
image building and linux-system-roles/tox-lsr#213
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by Sourcery
Enhance get_ostree_data.sh to not only list packages but also collect and format repository configurations in various formats (json, yaml, raw, toml) using shared suffix logic.
New Features:
Enhancements: